-
-
Notifications
You must be signed in to change notification settings - Fork 91
Implement Quotes Board #1029
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Implement Quotes Board #1029
Conversation
application/src/main/java/org/togetherjava/tjbot/features/Features.java
Outdated
Show resolved
Hide resolved
application/src/main/java/org/togetherjava/tjbot/features/basic/CoolMessagesBoardManager.java
Outdated
Show resolved
Hide resolved
application/src/main/java/org/togetherjava/tjbot/features/basic/CoolMessagesBoardManager.java
Outdated
Show resolved
Hide resolved
application/src/main/java/org/togetherjava/tjbot/features/basic/CoolMessagesBoardManager.java
Outdated
Show resolved
Hide resolved
application/src/main/java/org/togetherjava/tjbot/features/basic/CoolMessagesBoardManager.java
Outdated
Show resolved
Hide resolved
application/src/main/java/org/togetherjava/tjbot/features/basic/CoolMessagesBoardManager.java
Outdated
Show resolved
Hide resolved
application/src/main/java/org/togetherjava/tjbot/features/basic/CoolMessagesBoardManager.java
Outdated
Show resolved
Hide resolved
4a68771
to
bc3b7d6
Compare
35ba891
to
47cdcb2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add a message about config changes on main pr message, moderators said its hard to keep track of it otherwise
application/src/main/java/org/togetherjava/tjbot/features/basic/CoolMessagesBoardManager.java
Outdated
Show resolved
Hide resolved
application/src/main/java/org/togetherjava/tjbot/features/basic/CoolMessagesBoardManager.java
Outdated
Show resolved
Hide resolved
47cdcb2
to
6d86c8f
Compare
ed3dbd5
to
59dc245
Compare
59dc245
to
35abd02
Compare
maybe it makes more sense to implement this with the new forward feature in discord? |
35abd02
to
7b19538
Compare
@SquidXTV This is now a feature! With the latest changes, we are now forwarding messages instead of embedding messages. Good suggestion :) |
bbde421
to
7d839d6
Compare
Good job <3 |
Thank you! I have updated the PR description. Thanks for pointing it out! |
application/src/main/java/org/togetherjava/tjbot/config/CoolMessagesBoardConfig.java
Outdated
Show resolved
Hide resolved
application/src/main/java/org/togetherjava/tjbot/features/basic/CoolMessagesBoardManager.java
Outdated
Show resolved
Hide resolved
application/src/main/java/org/togetherjava/tjbot/features/basic/CoolMessagesBoardManager.java
Outdated
Show resolved
Hide resolved
application/src/main/java/org/togetherjava/tjbot/features/basic/CoolMessagesBoardManager.java
Outdated
Show resolved
Hide resolved
application/src/main/java/org/togetherjava/tjbot/features/basic/CoolMessagesBoardManager.java
Outdated
Show resolved
Hide resolved
application/src/main/java/org/togetherjava/tjbot/features/basic/CoolMessagesBoardManager.java
Outdated
Show resolved
Hide resolved
application/src/main/java/org/togetherjava/tjbot/features/basic/CoolMessagesBoardManager.java
Outdated
Show resolved
Hide resolved
9f303cd
to
8407d3f
Compare
gonna help with this |
For this feature, the version of JDA had to be bumped to 5.1.2
8407d3f
to
fb4fb5d
Compare
* minimumReactions-5, star symbol instead of encoding * requests changes by zabuzard except for moving getBoardChannel down and markMessageAsProcessed * Following JavaDocs guidelines of making the first letter capital * code refactoring * refactor: use correct method for reactionsCount It turns out that for each event fired, every *single* damn time, messageReaction.hasCount() would always return false. No matter what. Terrible documentation from JDA's side. As a result, because of the ternary operator: messageReaction.hasCount() ? messageReaction.getCount() + 1 : 1 the result of `reactionsCount` would always end up holding the value of one. In the following changes, we use `messageReaction.retrieveUsers()` to get a list of the people reacted, get a `Stream<User>` from that and get its count. Much more reliable this way and it also happens to be more readable. Signed-off-by: Chris Sdogkos <work@chris-sdogkos.com> Co-authored-by: Chris Sdogkos <work@chris-sdogkos.com> Co-authored-by: Surya Tejess <74978874+suryatejess@users.noreply.github.com>
Since 1ade409 (refactor: code review addressed by Zabuzard, 2025-06-28) primarily contains a generaly vague JavaDoc describing what the `QuoteBoardForwarder.java` class is doing, a more descriptive one replaces it. Signed-off-by: Chris Sdogkos <work@chris-sdogkos.com>
@@ -175,5 +175,10 @@ | |||
"fallbackChannelPattern": "java-news-and-changes", | |||
"pollIntervalInMinutes": 10 | |||
}, | |||
"coolMessagesConfig": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please rename it here as well, thanks
@@ -48,6 +48,7 @@ public final class Config { | |||
private final RSSFeedsConfig rssFeedsConfig; | |||
private final String selectRolesChannelPattern; | |||
private final String memberCountCategoryPattern; | |||
private final QuoteBoardConfig coolMessagesConfig; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rename here as well
required = true) String selectRolesChannelPattern, | ||
@JsonProperty(value = "coolMessagesConfig", | ||
required = true) QuoteBoardConfig coolMessagesConfig) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rename here as well
@@ -135,6 +138,7 @@ private Config(@JsonProperty(value = "token", required = true) String token, | |||
this.featureBlacklistConfig = Objects.requireNonNull(featureBlacklistConfig); | |||
this.rssFeedsConfig = Objects.requireNonNull(rssFeedsConfig); | |||
this.selectRolesChannelPattern = Objects.requireNonNull(selectRolesChannelPattern); | |||
this.coolMessagesConfig = Objects.requireNonNull(coolMessagesConfig); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rename here as well
/** | ||
* The configuration of the cool messages config. | ||
* | ||
* @return configuration of cool messages config | ||
*/ | ||
public QuoteBoardConfig getCoolMessagesConfig() { | ||
return coolMessagesConfig; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rename here as well
id also appreciate if we add a second sentence to the javadoc here to summarize the feature quickly. perhaps:
The configuration of the quote board feature. Quotes user selected messages.
* @param reactionEmoji the emoji with which users should react to | ||
*/ | ||
public QuoteBoardConfig { | ||
Objects.requireNonNull(boardChannelPattern); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is that possibly missing checks on the other params as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like the checks for minimumReactions
, and reactionEmoji
is not required in the record constructor as they'd already be configured in the config template
But there is a possibility that there might not be a channel on the server with the name contained by theboardChannelPattern
variable and hence we need it here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
they are all (rightfully) marked as required though. so we need to check they are actually set to a non null value
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sounds fair
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since minimumReactions
is an integer, i dont think we need to perform a check for it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
aye. but the other one is a string 👍
import java.util.regex.Pattern; | ||
|
||
/** | ||
* Listens for reaction-add events and turns popular messages into “quotes”. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no smart-quotes please. just "
@Override | ||
public void onMessageReactionAdd(MessageReactionAddEvent event) { | ||
final MessageReaction messageReaction = event.getReaction(); | ||
boolean isCoolEmoji = messageReaction.getEmoji().equals(triggerReaction); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
id rename that to sth like isTriggerEmoji
} | ||
|
||
final int reactionsCount = (int) messageReaction.retrieveUsers().stream().count(); | ||
if (isCoolEmoji && reactionsCount >= config.minimumReactions()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
personally id favor early-return to reduce the nesting here.
boolean isCoolEmoji = messageReaction.getEmoji().equals(triggerReaction); | ||
long guildId = event.getGuild().getIdLong(); | ||
|
||
if (hasAlreadyForwardedMessage(event.getJDA(), messageReaction)) { | ||
return; | ||
} | ||
|
||
final int reactionsCount = (int) messageReaction.retrieveUsers().stream().count(); | ||
if (isCoolEmoji && reactionsCount >= config.minimumReactions()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there is no need to do the hasAlreadyForwardedMessage
and the reactionsCount
computation if (!isCoolEmoji)
. add an early-return.
c3c1579
to
a6085db
Compare
Closes #1027.
Screenshots
Configuration changes
coolMessagesConfig.minimumReaction
for the target message to
be considered to the board channel.
coolMessagesConfig.boardChannelPattern
that should be quoted get posted to.
"quotes"
coolMessagesConfig.reactionEmoji
"U+2B50"